(closes #1312) fix scalar array functionality - #3520
Conversation
mo-alistairp
added
bug
in progress
LFRic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3520 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 399 399
Lines 56462 56473 +11
=========================================
+ Hits 56462 56473 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…hub.com:stfc/PSyclone into 1312_fix_scalar_array_functionality_invoke_call
|
I've taken this on from Alistair as he has moved roles. This is ready for a first look now. It's quite LFRic heavy so one for @sergisiso or @hiker. (Although I'm now scared that Sergi will use Codex...) We now have an example of the functionality that compiles and links so are in much better shape. @christophermaynard or @stevemullerworth - the one thing I omitted to ask Alistair for was a 'science' contact to help with testing of this functionality. Could you help here? |
sergisiso
left a comment
sergisiso
left a comment
There was a problem hiding this comment.
@arporter The changes look good and the generated code looks correct.
One problem I see is that the array_scalars handling is missing in some places for instance in: src/psyclone/domain/lfric/kernel/meta_args_metadata.py:81 and the kern_call_invoke_arg_list.py that I mentioned in an inline comment. I am wondering now if this is a matter of the old/new metadata path. In which case I might prefer to keep the new path as is, since I have changed it substancially in another PR, but we may want to document the divergence.
|
|
||
| compile: transform | ||
| @echo "No compilation supported for lfric/eg1" | ||
| single_invoke_alg.f90 single_invoke_psy.f90: single_invoke.x90 |
There was a problem hiding this comment.
The only comment from Codex was that modifying ../code/testkern_mod.F90 should retrigger this target as the psy-layer could be different. It is right (and very smart again, the kernel is not explicitly mentioned here so it knows what single_invoke.x90 have and that kernel metadata changes can produce changes in single_invoke_psy.f90 which is the output of this target).
However, this is a mistake that we have consistently across all examples and I would not explictly mention the specific kernel in the Makefile rule, which would fix the issue, but this would duplicate what is encoded inside the alg file. We could maybe mention the whole directory since it is explictly mentioned in the command, but make it worse the "-d" parameter is recursive. So maybe the right solution is something like https://stackoverflow.com/questions/25005637/makefile-rule-depend-on-directory-content-changes ?
I don't know. I will let you decide if its worth fixing, or leave it be, or just create a separate issue.
| def generate(self, var_accesses=None): | ||
| ''' Ensures that our internal lists of arguments of various | ||
| types are reset (as calling generate() populates them) before calling | ||
| this method in the parent class. | ||
|
|
||
| :param var_accesses: optional VariablesAccessMap instance to store | ||
| the information about variable accesses. | ||
| :type var_accesses: :py:class:`psyclone.core.VariablesAccessMap` | ||
| ''' | ||
| self._fields = [] | ||
| self._scalars = [] | ||
| self._qr_objects = [] | ||
| self._operators = [] | ||
| self._halo_depth = None | ||
| super().generate(var_accesses) | ||
|
|
||
| def scalar(self, | ||
| scalar_arg: "LFRicKernelArgument", | ||
| var_accesses: Optional[VariablesAccessMap] = None | ||
| ) -> None: | ||
| ''' | ||
| Add the necessary argument for a scalar quantity as well as an | ||
| appropriate Symbol to the SymbolTable. |
There was a problem hiding this comment.
Apparently we don't keep a separate list of scalar_arrays arguments here, so Codex recons they still go through the scalar method and creates a scalar DataSymbol, ignoring scalar_arg._array_ndims
However the generated code in the output is correct. I am always confuse by this class but I cannot see scalar arrays handled anywhere. How come this doesn't matter?
| " subroutine invoke_0(f1, real_array, logical_array, integer_array, " | ||
| "dims_integer_array, a, f2, f3, f4, b, dims_real_array, " | ||
| "dims_logical_array, dims_integer_array_1)\n" | ||
| "dims_integer_array, a, f2, f3, f4, b)\n" |
There was a problem hiding this comment.
This "dim_integer_array" here had me confused until I read the algorithm file which has:
! Include dims_integer_array as a scalar value to check that the
! generated code names do not clash
call invoke( &
testkern_scalar_array_type(f1, real_array, logical_array, integer_array, dims_integer_array), &
Maybe we can repeat the comment here to make clear why we recieve this name.
3 participants
The
gh_scalar_arrayfunctionality is not working as intended (#1312). The generated kernel subroutine includes an array of the extents of the dimensions of the scalar array in the the list of arguments. This isn't being generated in the corresponding invoke call. As a result the mismatch in arguments causes build errors whengh_scalar_arrayis used.This was likely because
kern_call_invoke_arg_list.pywas missed. This wasn't picked up by testing because the algorithm layer doesn't get compiled. As a way to verify that this functionality does work I'll also add an example usinggh_scalar_arraythat gets compiled to ensure that it builds correctly. I'll probably also add a bit more documentation to aid users.